Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C#] Enable nullability for variant classes #82983

Merged

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 7, 2023

Another minor update on the road to GodotSharp being nullable by default. In a similar vein to #82980, this focuses only on Variant types, but the class-equivalents instead. Still mostly small changes, but this is comparatively more involved than the structs, so it warrants its own PR

@Repiteo Repiteo requested a review from a team as a code owner October 7, 2023 21:43
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping annotate the codebase for nullability.

I feel like a lot of these cases shouldn't allow null. It looks like you might have just added ? to the parameters that are checked for nullability, but that doesn't necessarily mean a null value is acceptable.

I appreciate this being a separate PR, because I think it may take some time to decide the nullability of some of these parameters.

modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/NodePath.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/NodePath.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs Outdated Show resolved Hide resolved
@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 8, 2023

It looks like you might have just added ? to the parameters that are checked for nullability, but that doesn't necessarily mean a null value is acceptable.

Oh! Well then, that's a misconception on my end. In that case I would assume the null checks being for non-null types are for when they're called via a null-oblivious environment?

@raulsntos
Copy link
Member

Yes or when the user intentionally ignores the warning or uses the null forgiving operator (!), the documentation reads:

Nullable reference types are a compile time feature. That means it's possible for callers to ignore warnings, intentionally use null as an argument to a method expecting a non nullable reference. Library authors should include runtime checks against null argument values.

So, nullable types can be used to document if the parameter allows null, but the implementation should still check anyway. Of course, this only applies to public API, non-public API doesn't have to check because we should be able to ensure we are not calling it wrong.

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch 2 times, most recently from d40d02d to c26f35e Compare October 8, 2023 14:49
@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 8, 2023

Made some expansions to GodotObject, namely the extension class, and after checking what other classes might've been affected I realized the rabbit-hole effect of this process. Now DelegateUtils.cs and ScriptManagerBridge.cs recieved minor updates to account for the new nullability checks (plus a fix in the latter that I recall we talked about at some point, but never actually put into a proper commit). If those seem out of the scope of this PR, I'll revert them

After this, I'll probably make a draft PR that just converts everything to nullable, and future PRs will simply piecemeal from that

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch from c26f35e to b7d9c4f Compare October 8, 2023 16:35
@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 8, 2023

Added null checks to the typed collection constructors taking their untyped collection equivalents, as them not being there before seems to be an oversight (_underlyingArray is assumed to never be null)

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch from b7d9c4f to 8e74445 Compare October 9, 2023 14:27
@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch 2 times, most recently from 1ea013c to 1455224 Compare October 9, 2023 16:39
@akien-mga akien-mga changed the title C#: class variants to nullable environment C#: Class variants to nullable environment Oct 9, 2023
@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch from 1455224 to 63f14c4 Compare October 10, 2023 15:27
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference types should implement IEquatable<T?>, that means NodePath should implement IEquatable<NodePath?> but right now it implements IEquatable<NodePath> (notice the missing ?).

@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch from 63f14c4 to 21b390a Compare October 14, 2023 14:38
@Repiteo Repiteo force-pushed the c#-godotsharp-nullable-classes branch 2 times, most recently from 6443844 to 4effd95 Compare December 1, 2023 16:33
@AThousandShips AThousandShips changed the title C#: Class variants to nullable environment [C#] Enable nullability for variant classes Dec 1, 2023
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@raulsntos raulsntos removed this from the 4.x milestone Dec 19, 2023
@raulsntos raulsntos added this to the 4.3 milestone Dec 19, 2023
@YuriSizov YuriSizov merged commit ef79e5d into godotengine:master Dec 19, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@Repiteo Repiteo deleted the c#-godotsharp-nullable-classes branch December 19, 2023 19:39
@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 21, 2023

Builds for the .NET version on the build server fail with these errors, which I believe are related to this PR:

modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs(87,42): error CS0103: The name 'from' does not exist in the current context [modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]
modules/mono/glue/GodotSharp/GodotSharp/Core/Array.cs(1145,42): error CS0103: The name 'from' does not exist in the current context [modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]
modules/mono/glue/GodotSharp/GodotSharp/Core/Dictionary.cs(564,42): error CS0103: The name 'from' does not exist in the current context [modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]
modules/mono/glue/GodotSharp/GodotSharp/Core/NodePath.cs(141,42): error CS0103: The name 'from' does not exist in the current context [modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]

And also with

modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs(244,90): error CS0103: The name 'ptr' does not exist in the current context [modules/mono/glue/GodotSharp/GodotSharp/GodotSharp.csproj]

which is related to #85975.

Fix: #86373.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants